-
Notifications
You must be signed in to change notification settings - Fork 28.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SPARK-5143 [BUILD] [WIP] spark-network-yarn 2.11 depends on spark-network-shuffle 2.10 #4876
Conversation
Test build #28234 has started for PR 4876 at commit
|
Test build #28234 has finished for PR 4876 at commit
|
Test PASSed. |
| xargs -I {} sed -i -e 's|\(artifactId.*\)_2.11|\1_2.10|g' {} | ||
|
||
# Also update <scala.binary.version> in parent POM | ||
sed -i -e 's|<scala\.binary\.version>2.11<|<scala.binary.version>2.10<|1' pom.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, but can't this be done through profiles in maven ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is how the POMs will be read by tools that just download the POMs to understand dependencies. They read the 'default' config. The alternative would be to edit the file to move <activeByDefault>
to the other Scala profile, though that seemed harder.
I am not a sed wizard, was just going through the man page. It does not say anything about -e being POSIX or non POSIX - sadly!. I do not have a mac, so no clues about the problem you stated. |
Don't worry about |
Test build #28261 has started for PR 4876 at commit
|
@ScrapCodes I updated the script to use more 'standard' |
Test build #28262 has started for PR 4876 at commit
|
sed let's you use any separator of your choice, I can not link you to documentation which validates this claim right away. I am not sure about its POSIX compliance either, (I can look it up in free time though.). For now, I don't see any harm in changing '|' '/' character. removing the 'g' flag at the end ensure sed only replaces once. |
I did not run the script personally, but from the code it looks okay(as in harmless) to me. Sorry, I am not yet clear on how useful the addition of the last line is going to be. Because from your comment it is not clear
|
@ScrapCodes Right now, the published POMs are incorrect for Scala 2.11, because the parent POM defines Of course, the POM has a profile to switch this value for Scala 2.11. But this has no effect on how anyone consuming these POMs parse the POMs. They just read the default value instead that we've published for Scala 2.11. That is, when any tool (Maven etc) reads these POMs, they do so as if without any special flags, vars, profiles activated. So we also have to edit this value manually when making the 2.11 POM. It's not a problem with tools; the dependencies described by the Spark Scala-2.11 POMs are wrong as a result of this, and any tool that reads them correctly reads the incorrect dependencies. |
Test build #28261 has finished for PR 4876 at commit
|
Test PASSed. |
Test build #28262 has finished for PR 4876 at commit
|
Test PASSed. |
Hmm.. I was trusting our effective pom trick was working properly. Forgot
|
Based on your comments and this issue I am going to take a deeper look tomorrow again. Meanwhile if you are rushed with respect to this PR. Looks like a good immediate solution. |
I commented on the JIRA. This LGTM as an immediate fix - clearly the property is not correct in the published 2.11 poms. |
…work-shuffle 2.10 Update `<scala.binary.version>` prop in POM when switching between Scala 2.10/2.11 ScrapCodes for review. This `sed` command is supposed to just replace the first occurrence, but it replaces them all. Are you more of a `sed` wizard than I? It may be a GNU/BSD thing that is throwing me off. Really, just the first instance should be replaced, hence the `[WIP]`. NB on OS X the original `sed` command here will create files like `pom.xml-e` through the source tree though it otherwise works. It's like `-e` is also the arg to `-i`. I couldn't get rid of that even with `-i""`. No biggie. Author: Sean Owen <[email protected]> Closes #4876 from srowen/SPARK-5143 and squashes the following commits: b060c44 [Sean Owen] Oops, fixed reversed version numbers! e875d4a [Sean Owen] Add note about non-GNU sed; fix new pom.xml update to work as intended on GNU sed 703e1eb [Sean Owen] Update scala.binary.version prop in POM when switching between Scala 2.10/2.11 (cherry picked from commit 7ac072f) Signed-off-by: Patrick Wendell <[email protected]>
Update
<scala.binary.version>
prop in POM when switching between Scala 2.10/2.11@ScrapCodes for review. This
sed
command is supposed to just replace the first occurrence, but it replaces them all. Are you more of ased
wizard than I? It may be a GNU/BSD thing that is throwing me off. Really, just the first instance should be replaced, hence the[WIP]
.NB on OS X the original
sed
command here will create files likepom.xml-e
through the source tree though it otherwise works. It's like-e
is also the arg to-i
. I couldn't get rid of that even with-i""
. No biggie.